Skip to content

Add Image Prefetching for Click to expand Images #61107

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 18 commits into
base: trunk
Choose a base branch
from

Conversation

Takshil-Kunadia
Copy link
Contributor

@Takshil-Kunadia Takshil-Kunadia commented Apr 25, 2024

PR for #60883

What?

Prefetch full-resolution image to speed up display of Click to expand images.
see #60883 for more details

Why?

Hovering over a click-to-expand image block does not proactively attempt to prefetch the full-scale image in Lighbox. This can result in a prolonged period in which the low-resolution scaled-up image is displayed in the lightbox.

How?

  • Implement prefetchImage action that prefetches an image and keeps it in the browser cache.
  • Then utilising the above add pointer event directives that trigger it on pointerup & pointerdown.

Testing Instructions

  1. Create a post with a click to expand image on it.
  2. Open Dev tools and throttle network to slow 3G and notice the network tab.
  3. Hover over the image, this will trigger the image to load.
  4. When the loading is complete, click the image for it to load in full resolution without any delay or loading.

Testing Instructions for Keyboard

Screenshots or screencast

Screenshot 2024-04-25 at 10 18 51 PM

Copy link

github-actions bot commented Apr 25, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: Takshil-Kunadia <[email protected]>
Co-authored-by: luisherranz <[email protected]>
Co-authored-by: westonruter <[email protected]>
Co-authored-by: artemiomorales <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@Takshil-Kunadia Takshil-Kunadia force-pushed the feature/prefetch-lightbox-images branch from 68fcd2d to 5c10b84 Compare April 25, 2024 16:58
@artemiomorales
Copy link
Contributor

@Takshil-Kunadia Thanks for creating this PR! I've left a comment on the issue to loop a few other folks into the discussion who may have thoughts on how to best proceed. In short, we previously had prefetching on hover but removed it. Since then, there's been more discussion around this, so perhaps may be good to add this back.

@shail-mehta shail-mehta added [Type] Enhancement A suggestion for improvement. [Block] Image Affects the Image Block labels Jun 15, 2024
@Takshil-Kunadia Takshil-Kunadia force-pushed the feature/prefetch-lightbox-images branch from 5c10b84 to e6133b0 Compare November 15, 2024 21:07
@michalczaplinski michalczaplinski removed their request for review December 5, 2024 17:38
@Takshil-Kunadia Takshil-Kunadia force-pushed the feature/prefetch-lightbox-images branch from e6133b0 to 7593d56 Compare June 17, 2025 15:52
@Takshil-Kunadia
Copy link
Contributor Author

Thanks @artemiomorales ! for the context and for looping the stakeholders in the discussion! I've updated the code to reflect the direction discussed in the issue. Happy to adjust further based on any additional feedback 😄

@Takshil-Kunadia Takshil-Kunadia force-pushed the feature/prefetch-lightbox-images branch from 27aa823 to 6dd8ed5 Compare June 18, 2025 05:05
@westonruter
Copy link
Member

I'm just realizing a problem here with the overall image lightbox: what if the original src image is too large for the viewport when the image is expanded (e.g. on mobile)? Shouldn't the expanded lightbox image copy the srcset as well? If this is the case, prefetching the src wouldn't necessarily be correct since a smaller size could be more appropriate. This could be achieved simply by copying the srcset of the original image to lightbox image and also adding it to the imagesrcset of the prefetch link. In addition to this, an appropriate sizes attribute would need to be created specifically for the lightbox image, although the browser default of 100vw would probably be correct for the lightbox image. In this way, the lightbox would avoid downloading a larger image than needed, and this further speeding up displaying of the expanded image.

@Takshil-Kunadia Takshil-Kunadia force-pushed the feature/prefetch-lightbox-images branch from 3e26db9 to 3ab6bc0 Compare June 22, 2025 07:52
@Takshil-Kunadia
Copy link
Contributor Author

Takshil-Kunadia commented Jun 22, 2025

Thanks! @westonruter for looking into this 🙇🏻‍♂️ and yes good catch! it makes sense to have responsive images for lightbox images to avoid unnecessarily large downloads, especially on smaller viewports.

One thing I’d like to highlight though is that imagesrcset and imagesizes are supported with preload, not with prefetch afaik (reference: https://web.dev/articles/preload-responsive-images). This would slightly deviate from the original approach discussed in the issue which was on using prefetch as part of speculative loading.

That said, I’ve implemented the suggested preload approach in d7f2e41

Copy link
Member

@luisherranz luisherranz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, just a couple of comments 🙂

Comment on lines 222 to 226
const srcset = imageMetadata.lightboxSrcset;
if ( srcset ) {
imageLink.setAttribute( 'imagesrcset', srcset );
imageLink.setAttribute( 'imagesizes', '100vw' );
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't enlargedSrcset and enlargedSizes be used here?

Copy link
Contributor Author

@Takshil-Kunadia Takshil-Kunadia Jun 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use the getter functions, but for that we need to temporarily set the currentImageId to use them and then reset the Id when we have preloaded the image.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for that we need to temporarily set the currentImageId to use them and then reset the Id when we have preloaded the image

I don't think it would be a good approach, as there can be more than one image preloading at the same time.

Why don't you just use imageMetadata.lightboxSizes || '100vw' instead of just '100vw' here?

Copy link
Contributor Author

@Takshil-Kunadia Takshil-Kunadia Jun 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was how it was implemented previously. I changed it in cab1562 to use the getter fns.
Reverted this commit ↩️ along with the dynamic lightboxSizes change

Comment on lines 33 to 64
/**
* Returns the appropriate src URL for an image
*
* @param {Object} imageMetadata Image metadata object
* @return {string} The source URL
*/
function getImageSrc( imageMetadata ) {
return (
imageMetadata.uploadedSrc ||
'data:image/gif;base64,R0lGODlhAQABAAD/ACwAAAAAAQABAAACADs='
);
}

/**
* Returns the appropriate srcset for an image
*
* @param {Object} imageMetadata Image metadata object
* @return {string} The srcset value
*/
function getImageSrcset( imageMetadata ) {
return imageMetadata.lightboxSrcset || '';
}

/**
* Returns the appropriate sizes attribute for an image
*
* @param {Object} imageMetadata Image metadata object
* @return {string} The sizes value, defaulting to 100vw
*/
function getImageSizes( imageMetadata ) {
return imageMetadata.lightboxSizes || '100vw';
}
Copy link
Member

@westonruter westonruter Jun 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor improvement, to add typing for the passed prop:

Suggested change
/**
* Returns the appropriate src URL for an image
*
* @param {Object} imageMetadata Image metadata object
* @return {string} The source URL
*/
function getImageSrc( imageMetadata ) {
return (
imageMetadata.uploadedSrc ||
'data:image/gif;base64,R0lGODlhAQABAAD/ACwAAAAAAQABAAACADs='
);
}
/**
* Returns the appropriate srcset for an image
*
* @param {Object} imageMetadata Image metadata object
* @return {string} The srcset value
*/
function getImageSrcset( imageMetadata ) {
return imageMetadata.lightboxSrcset || '';
}
/**
* Returns the appropriate sizes attribute for an image
*
* @param {Object} imageMetadata Image metadata object
* @return {string} The sizes value, defaulting to 100vw
*/
function getImageSizes( imageMetadata ) {
return imageMetadata.lightboxSizes || '100vw';
}
/**
* Returns the appropriate src URL for an image.
*
* @param {string} uploadedSrc - Full size image src.
* @return {string} The source URL.
*/
function getImageSrc( { uploadedSrc } ) {
return (
uploadedSrc ||
'data:image/gif;base64,R0lGODlhAQABAAD/ACwAAAAAAQABAAACADs='
);
}
/**
* Returns the appropriate srcset for an image.
*
* @param {string} lightboxSrcset - Image srcset.
* @return {string} The srcset value.
*/
function getImageSrcset( { lightboxSrcset } ) {
return lightboxSrcset || '';
}
/**
* Returns the appropriate sizes attribute for an image.
*
* @param {string} lightboxSizes - Image responsive sizes attribute.
* @return {string} The sizes value, defaulting to 100vw.
*/
function getImageSizes( { lightboxSizes } ) {
return lightboxSizes || '100vw';
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made the changes ✅

Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside from the minor suggestion in #61107 (comment) this looks good to me. It is successfully preloading the image prior to opening it in the lightbox.

It can be hard to see, but in the before case the image is a bit blurry while the full size image is loaded:

Before After
without-preload.mov
with-preload.mov

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Image Affects the Image Block [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants